bpo-29757: don't swallows errors in the `socket.create_connection()` utility loop #562

ankostis · 2017-03-08T14:43:58Z

Context

The utility method socket.create_connection() currently works like that:

resolve the destination-address into one or more IP(v4 & v6) addresses;
loop on each IP address and stop to the 1st one to work;
if none works, re-raise the last error.

The problem

So currently the loop in socket.create_connection() ignores all intermediate errors and reports only the last connection failure, which might be irrelevant. For instance, when both IPv4 & IPv6 networks are supported, usually the last address is a IPv6 address and it frequently fails with an irrelevant error - the actual cause have already been ignored.

Possible solutions

To facilitate network debugging, there are at least 3 options:

a. log each failure as it happens, but that would get the final failure twice: once as a (warning?) message, and once as an exception .
b. collect all failures and log them only when connection fails, but that might miss important infos to the user;
c. collect and return all failures in list attached to the raised exception, which also might miss important infos.

open questions

Q1. A question for all cases is what logging "means" to use: the warnings or logging module?
Q2. And if logging is chosen, log them in 'DEBUG' or 'WARNING' level?
Q3. Finally, cases (b) & (c) need an answer whether to log collected errors in the case that connection passes.

The 1st part of case (c) is implemented in this PR.

the-knights-who-say-ni · 2017-03-08T14:43:59Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

If you don't have an account on b.p.o, please create one
Make sure your GitHub username is listed in "Your Details" at b.p.o
If you have not already done so, please sign the PSF contributor agreement. The "bugs.python.org username " requested by the form is the "Login name" field under "Your Details".
If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

mention-bot · 2017-03-08T14:44:00Z

@ankostis, thanks for your PR! By analyzing the history of the files in this pull request, we identified @freddrake, @ethanfurman, @asvetlov, @giampaolo and @cf-natali to be potential reviewers.

brettcannon · 2017-03-08T19:03:46Z

Could you open an issue to discuss this, @ankostis ? We prefer to discuss design and such on bugs.python.org so that GitHub is only used for code review.

ankostis · 2017-03-08T21:03:09Z

Issue 29757.

ankostis · 2017-03-08T21:04:54Z

By the way my CLA has been signed.

brettcannon · 2017-03-08T21:36:26Z

Thanks. And just in case you didn't notice, Travis failed (not sure if it's because of the PR changes).

ankostis · 2017-03-08T21:46:38Z

Thanks, I will look at it tomorrow.

brettcannon · 2017-03-08T23:04:47Z

@ankostis you can also wait for any discussion to be had about the issue before you put in more time in case this isn't accepted for some reason.

ankostis · 2017-03-08T23:11:43Z

@brettcannon grateful for your care to contributors like me.

Anyhow, I did some changes on the OP regarding what I tried to implement and what I consider as possible solutions & open questions.

1st1 · 2017-03-09T17:18:41Z

Lib/socket.py

@@ -714,12 +714,12 @@ def create_connection(address, timeout=_GLOBAL_DEFAULT_TIMEOUT,
            return sock

        except error as _:


Please don't use _, it would look much better with a normal variable name such as e.

Please don't use one letter name like "e" :-) Use "error", "exc" or whatever else.

Done in ~~ec887c0c3~~ (rebased on latest master).
Had to rewrite again (b39d4b1) to rename e --> ex (i like it also this way, just trying to preserve styles).

1st1 · 2017-03-09T17:18:55Z

Lib/socket.py

@@ -700,7 +700,7 @@ def create_connection(address, timeout=_GLOBAL_DEFAULT_TIMEOUT,
    """

    host, port = address
-    err = None
+    errs = []


errs -> errors

Done in ~~ec887c0c3~~ b39d4b1 (rebased on latest master).

1st1 · 2017-03-09T17:24:40Z

I'll post some feedback to bpo.

## Context The utility method `socket.create_connection()` currently works like that: 1. resolve the destination-address into one or more IP(v4 & v6) addresses; 2. loop on each IP address and stop to the 1st one to work; 3. if none works, re-raise the last error. ## The problem So currently the loop in `socket.create_connection()` ignores all intermediate errors and reports only the last connection failure, which might be irrelevant. For instance, when both IPv4 & IPv6 networks are supported, usually the last address is a IPv6 address and it frequently fails with an irrelevant error - the actual cause have already been ignored. ## Possible solutions & open questions To facilitate network debugging, there are at least 3 options: a. log each failure [as they happen](/python/cpython/blob/6f0eb93183519024cb360162bdd81b9faec97ba6/Lib/socket.py#L717), but that would get the final failure twice: once as a (warning?) message, and once as an exception . b. collect all failures and log them only when connection fails to collect the errors, but that might miss important infos to the user; c. collect and return all failures in list attached to the raised exception. A question for cases (a) & (b) is what logging "means" to use: the `warnings` or `logging` module? And if `logging` is chosen, log them in `'DEBUG'` or `'WARNING'` level? Case (c), implemented in this PR sidesteps the above questions.

ankostis · 2017-03-17T17:26:45Z

And just in case you didn't notice, Travis failed (not sure if it's because of the PR changes).

Now all TCs pass, because I re-raise the exact original error in case only a single one was collected.

But still, this PR is needs more work because of 2 issues:

Abuse of `OSError()` constructor

When more than one error is collected, I pass a string to the 1st arg to OSError() constructor (instead of an errno).
But I copied this from existing code.

Which errno should use there?

Notify user about collected errors on Success

The list of intermediate errors is lost in the case of success.
As suggested on the OP, this ~~may happen~~ can be corrected (in my order of preference):

with a new optional argument for the user to provide the list to collect the errors (it changes the API backward-compatibly but allows inspection of the errors);
with logging logs (more configurable than warning, but no inspection - only strings);
with warnings;
by doing nothing (swallowing error-messages is dangerous in the long term).

ankostis · 2017-03-17T17:27:17Z

@the-knights-who-say-ni I have signed CLA; is there some problem with it?

- Rename `_` and `errs` variable names. - Raise original error if a single one. - Improve a bit error message, but still no Errno provided for when multiple errors are included.

brettcannon · 2017-03-17T17:52:43Z

@ankostis I just checked the logs and it seems you didn't set your GitHub username on your bugs.python.org (i.e. the bot can't find any account on bugs.python.org that has the GitHub username "ankostis" set). Fix that and hopefully the bot will pick up that you signed the CLA.

ankostis · 2017-03-17T17:58:27Z

OK, my gh-username has been set.

brettcannon · 2017-03-17T18:03:19Z

@ankostis that worked!

ankostis · 2017-03-22T15:03:17Z

Any ideas which road to take?

And where should the discussion take place, here on on python-tracker?

brettcannon · 2017-03-22T16:43:36Z

@ankostis code review issues are discussed here, all other discussions (such as what the best approach is, etc.) should happen on the issue.

MaxwellDupre

I see that PEP 654 -- Exception Groups and except*
would be the best way to implement multiple exceptions here, but this will need to wait while being implemented. Or you could try the without except*
https://www.python.org/dev/peps/pep-0654/#programming-without-except

github-actions · 2022-03-10T00:08:44Z

This PR is stale because it has been open for 30 days with no activity.

kumaraditya303 · 2022-04-19T12:06:51Z

Superseded by #91586

the-knights-who-say-ni added the CLA not signed label Mar 8, 2017

ankostis mentioned this pull request Mar 8, 2017

feat(log): also report negotiations error in WARN level Anorov/PySocks#72

Merged

brettcannon added the type-feature A feature request or enhancement label Mar 8, 2017

brettcannon changed the title ~~feat(socket): don't swallows errors in socket.create_connection() utility loop~~ bpo-29757: don't swallows errors in the socket.create_connection() utility loop Mar 8, 2017

vstinner requested review from vstinner and 1st1 March 9, 2017 17:15

1st1 reviewed Mar 9, 2017

View reviewed changes

methane removed the CLA not signed label Mar 14, 2017

the-knights-who-say-ni added the CLA not signed label Mar 14, 2017

ankostis force-pushed the patch-1 branch from 5ccedec to ec887c0 Compare March 17, 2017 16:50

refact(socket): corrections according to python#562

b39d4b1

- Rename `_` and `errs` variable names. - Raise original error if a single one. - Improve a bit error message, but still no Errno provided for when multiple errors are included.

ankostis force-pushed the patch-1 branch from ec887c0 to b39d4b1 Compare March 17, 2017 17:40

brettcannon removed the CLA not signed label Mar 17, 2017

the-knights-who-say-ni added the CLA not signed label Mar 17, 2017

brettcannon removed the CLA not signed label Mar 17, 2017

the-knights-who-say-ni added the CLA signed label Mar 17, 2017

brettcannon added the awaiting review label Feb 21, 2018

MaxwellDupre reviewed Feb 7, 2022

View reviewed changes

github-actions bot added the stale Stale PR or inactive for long period of time. label Mar 10, 2022

ankostis mannequin mentioned this pull request Apr 15, 2022

The loop in utility socket.create_connection() swallows previous errors #73943

Closed

kumaraditya303 closed this Apr 19, 2022

jaraco pushed a commit that referenced this pull request Dec 2, 2022

Use Python 3.10.5 runtime (#562)

6134ede

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-29757: don't swallows errors in the `socket.create_connection()` utility loop #562

bpo-29757: don't swallows errors in the `socket.create_connection()` utility loop #562

ankostis commented Mar 8, 2017 •

edited

Loading

the-knights-who-say-ni commented Mar 8, 2017

mention-bot commented Mar 8, 2017

brettcannon commented Mar 8, 2017

ankostis commented Mar 8, 2017

ankostis commented Mar 8, 2017

brettcannon commented Mar 8, 2017 •

edited

Loading

ankostis commented Mar 8, 2017

brettcannon commented Mar 8, 2017

ankostis commented Mar 8, 2017 •

edited

Loading

1st1 Mar 9, 2017

vstinner Mar 9, 2017

ankostis Mar 17, 2017 •

edited

Loading

1st1 Mar 9, 2017

ankostis Mar 17, 2017 •

edited

Loading

1st1 commented Mar 9, 2017

ankostis commented Mar 17, 2017 •

edited

Loading

ankostis commented Mar 17, 2017

brettcannon commented Mar 17, 2017

ankostis commented Mar 17, 2017 •

edited

Loading

brettcannon commented Mar 17, 2017

ankostis commented Mar 22, 2017 •

edited

Loading

brettcannon commented Mar 22, 2017

MaxwellDupre left a comment

github-actions bot commented Mar 10, 2022

kumaraditya303 commented Apr 19, 2022

		@@ -714,12 +714,12 @@ def create_connection(address, timeout=_GLOBAL_DEFAULT_TIMEOUT,
		return sock

		except error as _:

bpo-29757: don't swallows errors in the socket.create_connection() utility loop #562

bpo-29757: don't swallows errors in the socket.create_connection() utility loop #562

Conversation

ankostis commented Mar 8, 2017 • edited Loading

Context

The problem

Possible solutions

open questions

the-knights-who-say-ni commented Mar 8, 2017

mention-bot commented Mar 8, 2017

brettcannon commented Mar 8, 2017

ankostis commented Mar 8, 2017

ankostis commented Mar 8, 2017

brettcannon commented Mar 8, 2017 • edited Loading

ankostis commented Mar 8, 2017

brettcannon commented Mar 8, 2017

ankostis commented Mar 8, 2017 • edited Loading

1st1 Mar 9, 2017

Choose a reason for hiding this comment

vstinner Mar 9, 2017

Choose a reason for hiding this comment

ankostis Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

1st1 Mar 9, 2017

Choose a reason for hiding this comment

ankostis Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

1st1 commented Mar 9, 2017

ankostis commented Mar 17, 2017 • edited Loading

Abuse of OSError() constructor

Notify user about collected errors on Success

ankostis commented Mar 17, 2017

brettcannon commented Mar 17, 2017

ankostis commented Mar 17, 2017 • edited Loading

brettcannon commented Mar 17, 2017

ankostis commented Mar 22, 2017 • edited Loading

brettcannon commented Mar 22, 2017

MaxwellDupre left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 10, 2022

kumaraditya303 commented Apr 19, 2022

bpo-29757: don't swallows errors in the `socket.create_connection()` utility loop #562

bpo-29757: don't swallows errors in the `socket.create_connection()` utility loop #562

ankostis commented Mar 8, 2017 •

edited

Loading

brettcannon commented Mar 8, 2017 •

edited

Loading

ankostis commented Mar 8, 2017 •

edited

Loading

ankostis Mar 17, 2017 •

edited

Loading

ankostis Mar 17, 2017 •

edited

Loading

ankostis commented Mar 17, 2017 •

edited

Loading

Abuse of `OSError()` constructor

ankostis commented Mar 17, 2017 •

edited

Loading

ankostis commented Mar 22, 2017 •

edited

Loading